Skip to content

Conversation

@Russole
Copy link
Contributor

@Russole Russole commented Nov 25, 2025

What changes were proposed in this pull request?

This PR improves and expands the test coverage of the PUT Bucket ACL operation in BucketEndpoint, ensuring correct handling of S3 ACL semantics under different request formats.

  • Added VALID_ACL_XML constant to provide a reusable, well-formed S3 ACL body for tests.

  • Added new test cases to fully validate the behavior of PUT /{bucket}?acl:

  • Missing ACL headers with a valid XML body should succeed.

  • Missing ACL headers and missing body should fail with a WebApplicationException caused by an OS3Exception with InvalidRequest.

  • PUT ACL on a non-existing bucket should throw an OS3Exception with NoSuchBucket.

  • Header-based ACL update using x-amz-grant-full-control should succeed.

  • Invalid XML body (non-XML input) should trigger an OS3Exception with InvalidRequest.

  • Malformed ACL grant header (e.g., missing "=") should produce an OS3Exception with InvalidArgument.

  • Removed two ineffective test cases (testBucketFailWithAuthHeaderMissing and testBucketFailWithInvalidHeader) because they used a try/catch without assertThrows, causing them to pass even when no exception was thrown.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-13932

How was this patch tested?

This patch includes new and enhanced unit tests in TestBucketPut to verify the updated behavior.
All tests were executed locally and passed.
Additionally, all CI pipelines passed after the patch was pushed.

Copy link
Contributor

@Gargi-jais11 Gargi-jais11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Russole for the patch.
Left some comments below.

Comment on lines 110 to 112

assertEquals("InvalidRequest", os3.getCode());
assertEquals(400, os3.getHttpCode());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here also we should use constant references consistently.

Suggested change
assertEquals("InvalidRequest", os3.getCode());
assertEquals(400, os3.getHttpCode());
assertEquals(INVALID_REQUEST.getCode(), os3.getCode());;
assertEquals(400, os3.getHttpCode());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. I've updated the code to use constant references consistently.

assertEquals(HTTP_NOT_FOUND, ex.getHttpCode());
assertEquals(MALFORMED_HEADER.getCode(), ex.getCode());
}
public void testAclWithMissingHeadersAndNoBody() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also have a test scenario where both headers and body are present, and check headers take precedence and the body is ignored.
Verify that the permissions set on the bucket is as per header and not according to the body.
testPutAclWithBothHeadersAndBody()

  • Mock header with one permission
  • Provide body with different permission (should be ignored)
  • Verify that READ permission was set (from header), not FULL_CONTROL (from body)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion, I have added a new test to cover this scenario:
In this test:

  • The header grants READ via x-amz-grant-read (id=owner-id),
  • The XML body (VALID_ACL_XML) grants FULL_CONTROL,
  • The call succeeds with HTTP 200, and We verify that an ACL entry for owner-id with ACCESS scope is present on the bucket, which is derived from the header.

Since putAcl() only takes the header-based path when any grant header is present (and skips the body in that case), this test documents that the header takes precedence over the body when both are provided.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case ...MissingHeadersAndNoBody is no longer necessary.

@Gargi-jais11
Copy link
Contributor

Few other scenarios are also possible, so do you have idea what will be the behaviour in the below scenarios?:

  • What if header exists but is empty and no body is there?
example:
when(mockHeaders.getHeaderString(S3Acl.GRANT_FULL_CONTROL))
      .thenReturn(""); //empty
Response resp = bucketEndpoint.put(bucketName, "acl", null);
  • Or what if header exists but has white space only and no body?
example:
when(mockHeaders.getHeaderString(S3Acl.GRANT_FULL_CONTROL))
      .thenReturn("  "); // whitespace
Response resp = bucketEndpoint.put(bucketName, "acl", null);

So in the above two case what should happen, create bucket should pass or fail ? And according do testing of these two scenarios as well.

@Russole Russole closed this Nov 27, 2025
@Russole Russole deleted the HDDS-13932 branch November 27, 2025 23:15
@Russole Russole reopened this Nov 28, 2025
@Russole
Copy link
Contributor Author

Russole commented Nov 28, 2025

Few other scenarios are also possible, so do you have idea what will be the behaviour in the below scenarios?:

  • What if header exists but is empty and no body is there?
example:
when(mockHeaders.getHeaderString(S3Acl.GRANT_FULL_CONTROL))
      .thenReturn(""); //empty
Response resp = bucketEndpoint.put(bucketName, "acl", null);
  • Or what if header exists but has white space only and no body?
example:
when(mockHeaders.getHeaderString(S3Acl.GRANT_FULL_CONTROL))
      .thenReturn("  "); // whitespace
Response resp = bucketEndpoint.put(bucketName, "acl", null);

So in the above two case what should happen, create bucket should pass or fail ? And according do testing of these two scenarios as well.

Thanks for pointing out these additional edge cases.

I checked the current implementation of BucketEndpoint.putAcl() (in particular getAndConvertAclOnBucket / getAndConvertAclOnVolume) and added tests to document the behavior for both scenarios:

  • Header exists but is empty (""), no body
when(mockHeaders.getHeaderString(S3Acl.GRANT_FULL_CONTROL))
    .thenReturn("");
Response resp = bucketEndpoint.put(bucketName, "acl", null);

In this case, StringUtils.isEmpty(value) returns true, so the helper returns an empty ACL list without throwing. The PUT ?acl call succeeds with HTTP 200 and effectively behaves as “no grants provided”.
This is covered by:

@Test
public void testPutAclWithEmptyGrantHeaderValue() throws Exception {
  assertEquals(200, resp.getStatus());
}
  • Header exists but has only whitespace (" "), no body
when(mockHeaders.getHeaderString(S3Acl.GRANT_FULL_CONTROL))
    .thenReturn("  ");
Response resp = bucketEndpoint.put(bucketName, "acl", null);

Here StringUtils.isEmpty(value) is false, so we attempt to parse the value.
Splitting on "=" produces a single part (part.length != 2), which triggers:

throw newError(S3ErrorTable.INVALID_ARGUMENT, acl);

So this scenario fails with OS3Exception(INVALID_ARGUMENT, 400).
This is covered by:

@Test
public void testPutAclWithWhitespaceGrantHeaderValue() throws Exception {
  OS3Exception ex = assertThrows(OS3Exception.class,
      () -> bucketEndpoint.put(bucketName, "acl", null));
  assertEquals(INVALID_ARGUMENT.getCode(), ex.getCode());
  assertEquals(400, ex.getHttpCode());
}

These two tests make the current behavior explicit and should help prevent regressions around header parsing for PUT ?acl.

@Russole Russole requested a review from Gargi-jais11 November 28, 2025 16:59
Copy link
Contributor

@Gargi-jais11 Gargi-jais11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Russole for updating the patch.
Just one minor comment. Overall LGTM.

Comment on lines +212 to +214
.orElseThrow(() -> new AssertionError("owner-id ACL not found"));

assertEquals("owner-id", ownerAcl.getName());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please also add an assert statement to verify the permission is READ (from header), not FULL_CONTROL (from body).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the requested assertions — the test now verifies that READ from the header is applied and FULL_CONTROL from the body is ignored.

@Russole Russole closed this Dec 1, 2025
@Russole Russole restored the HDDS-13932 branch December 1, 2025 22:13
@Russole Russole reopened this Dec 1, 2025
@Russole Russole requested a review from Gargi-jais11 December 1, 2025 22:14
@Gargi-jais11
Copy link
Contributor

@Russole there is no need to close the PR everytime you make changes. If you wish you can mark it as draft and after its done make it ready for review.

Copy link
Contributor

@Gargi-jais11 Gargi-jais11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patch LGTM!

@Russole
Copy link
Contributor Author

Russole commented Dec 2, 2025

Thank you for the clarification.
I’ll keep the PR open and use the Draft status while making updates moving forward.
Appreciate your guidance.

@Russole
Copy link
Contributor Author

Russole commented Dec 5, 2025

Just checking in kindly. Since the patch already has LGTM,
please feel free to let me know if any adjustments are needed —
I’m very happy to update it anytime. Thank you!

@chungen0126 chungen0126 changed the title HDDS 13932. Fix TestBucketPut by correcting test logic and updating test cases HDDS-13932. Fix TestBucketPut by correcting test logic and updating test cases Dec 8, 2025
Copy link
Contributor

@chungen0126 chungen0126 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Russole for the improvement. Left some comments.


InputStream body = new ByteArrayInputStream(VALID_ACL_XML.getBytes(StandardCharsets.UTF_8));

Response resp = bucketEndpoint.put(bucketName, "acl", body);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introduce a static string constant to replace all hardcoded instances of 'acl'.

InputStream body = new ByteArrayInputStream(VALID_ACL_XML.getBytes(StandardCharsets.UTF_8));

Response resp = bucketEndpoint.put(bucketName, "acl", body);
assertEquals(200, resp.getStatus());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assertEquals(200, resp.getStatus());
assertEquals(HTTP_OK, resp.getStatus());

Use HTTP_OK to replace 200.


Response resp = bucketEndpoint.put(bucketName, "acl", null);

assertEquals(200, resp.getStatus());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assertEquals(200, resp.getStatus());
assertEquals(HTTP_OK, resp.getStatus());

Use HTTP_OK to replace 200.

VALID_ACL_XML.getBytes(StandardCharsets.UTF_8));

Response resp = bucketEndpoint.put(bucketName, "acl", body);
assertEquals(200, resp.getStatus());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assertEquals(200, resp.getStatus());
assertEquals(HTTP_OK, resp.getStatus());

Use HTTP_OK to replace 200.


Response resp = bucketEndpoint.put(bucketName, "acl", null);

Assertions.assertEquals(200, resp.getStatus());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Assertions.assertEquals(200, resp.getStatus());
assertEquals(HTTP_OK, resp.getStatus());

Use HTTP_OK to replace 200.

OS3Exception os3 = (OS3Exception) cause;

assertEquals(INVALID_REQUEST.getCode(), os3.getCode());
assertEquals(400, os3.getHttpCode());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assertEquals(400, os3.getHttpCode());
assertEquals(HTTP_BAD_REQUEST, os3.getHttpCode());

Use HTTP_BAD_REQUEST to replace 400.

OS3Exception os3 = (OS3Exception) cause;

assertEquals(INVALID_REQUEST.getCode(), os3.getCode());
assertEquals(400, os3.getHttpCode());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assertEquals(400, os3.getHttpCode());
assertEquals(HTTP_BAD_REQUEST, os3.getHttpCode());

Use HTTP_BAD_REQUEST to replace 400.

() -> bucketEndpoint.put(bucketName, "acl", null));

assertEquals(INVALID_ARGUMENT.getCode(), ex.getCode());
assertEquals(400, ex.getHttpCode());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assertEquals(400, ex.getHttpCode());
assertEquals(HTTP_BAD_REQUEST, os3.getHttpCode());

Use HTTP_BAD_REQUEST to replace 400.

() -> bucketEndpoint.put(bucketName, "acl", null));

Assertions.assertEquals(INVALID_ARGUMENT.getCode(), ex.getCode());
Assertions.assertEquals(400, ex.getHttpCode());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Assertions.assertEquals(400, ex.getHttpCode());
assertEquals(HTTP_BAD_REQUEST, os3.getHttpCode());

Use HTTP_BAD_REQUEST to replace 400.

assertEquals(MALFORMED_HEADER.getCode(), ex.getCode());
}
public void testAclWithMissingHeaders() throws Exception {
bucketEndpoint.getClient().getObjectStore().createS3Bucket(bucketName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to create an S3 bucket before bucket put?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because putAcl() first calls getS3Bucket(bucketName).
If the bucket does not exist, it throws NO_SUCH_BUCKET and never reaches any ACL parsing logic.
To test the ACL behavior, the bucket must already exist.

Copy link
Contributor

@echonesis echonesis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Russole , left some comments.

OS3Exception ex = assertThrows(OS3Exception.class,
() -> bucketEndpoint.put(bucketName, "acl", null));

Assertions.assertEquals(INVALID_ARGUMENT.getCode(), ex.getCode());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Assertions.assertEquals(INVALID_ARGUMENT.getCode(), ex.getCode());
assertEquals(INVALID_ARGUMENT.getCode(), ex.getCode());

assertEquals is enough.

bucketEndpoint.getClient().getObjectStore().createS3Bucket(bucketName);

when(mockHeaders.getHeaderString(S3Acl.GRANT_FULL_CONTROL))
.thenReturn(" "); // whitespace only
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider introducing a static constant for the whitespace string to improve readability:

Suggested change
.thenReturn(" "); // whitespace only
private static final String WHITESPACE_ONLY = " ";
when(mockHeaders.getHeaderString(S3Acl.GRANT_FULL_CONTROL))
.thenReturn(WHITESPACE_ONLY);

@Russole
Copy link
Contributor Author

Russole commented Dec 8, 2025

Thanks @echonesis and @chungen0126 for the review and helpful comments — really appreciate your guidance! 🙏
I’ll update the tests accordingly.

" </Grant>" +
" </AccessControlList>" +
"</AccessControlPolicy>";
private static final String ACL = "acl";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you could use OzoneConsts.ACL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion!
I've removed the local "acl" constant and updated the tests to use OzoneConsts.ACL so that we rely on the shared Ozone constant instead of redefining it.

@Russole
Copy link
Contributor Author

Russole commented Dec 9, 2025

Thanks to @Gargi-jais11, @chungen0126, and @echonesis for the review and helpful feedback.

@Russole Russole requested a review from echonesis December 10, 2025 11:20
() -> bucketEndpoint.put(bucketName, OzoneConsts.ACL, null));

assertEquals(INVALID_ARGUMENT.getCode(), ex.getCode());
assertEquals(400, ex.getHttpCode());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assertEquals(400, ex.getHttpCode());
assertEquals(HTTP_BAD_REQUEST, ex.getHttpCode());

Use HTTP_BAD_REQUEST to replace 400.

@Russole Russole requested a review from echonesis December 15, 2025 14:42
Copy link
Contributor

@echonesis echonesis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Russole for updating the patch. LGTM!

@Gargi-jais11
Copy link
Contributor

@ivandika3 and @peterxcli Please take a look on the patch.

@Russole
Copy link
Contributor Author

Russole commented Jan 10, 2026

Thanks @Gargi-jais11 for the earlier ping.
Just following up to see if anyone could help review this PR.
@chungen0126 , would you have time to take a look when convenient?
Thanks!

Copy link
Contributor

@chungen0126 chungen0126 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 LGTM

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Russole for the patch and sorry for the conflicts. Can you please move the test cases to TestBucketAcl or TestBucketAclHandler? BucketEndpoint.putAcl no longer exists (code refactored into BucketAclHandler). TestBucketAcl adds the ?acl query param, so you can use BucketEndpoint.put there. Please check if existing test cases cover any of the new ones.

Comment on lines +103 to +104
Response resp = bucketEndpoint.putAcl(bucketName, body);
assertEquals(HTTP_OK, resp.getStatus());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we have helper method to simplify this.

assertSucceeds(() -> bucketEndpoint.put(bucketName, body));

assertEquals(HTTP_NOT_FOUND, ex.getHttpCode());
assertEquals(MALFORMED_HEADER.getCode(), ex.getCode());
}
public void testAclWithMissingHeadersAndNoBody() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case ...MissingHeadersAndNoBody is no longer necessary.

Comment on lines +268 to +272
OS3Exception ex = assertThrows(OS3Exception.class,
() -> bucketEndpoint.putAcl(bucketName, null));

assertEquals(INVALID_ARGUMENT.getCode(), ex.getCode());
assertEquals(HTTP_BAD_REQUEST, ex.getHttpCode());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we have helper to simplify this:

assertErrorResponse(INVALID_ARGUMENT, () -> bucketEndpoint.put(bucketName, null));

@adoroszlai adoroszlai marked this pull request as draft January 15, 2026 15:16
@adoroszlai adoroszlai changed the title HDDS-13932. Fix TestBucketPut by correcting test logic and updating test cases HDDS-13932. Add test cases for PUT bucket ACL Jan 15, 2026
@adoroszlai adoroszlai added test s3 S3 Gateway labels Jan 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

s3 S3 Gateway test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants